Add correct column metadata to all warning checks #1127

NJichev · 2024-04-29T15:27:01Z

Hey 👋, I was working on next-ls code actions from credo when we noticed that the column information is missing.
This branch is a follow up on #1126. The main use case is generating a correct code action if there are two warnings on the same line. Without the column information 2 different warnings will map to the same AST when searching for it.

I kinda picked the column arbitrarily, so ping me if we should take it from another AST node, but generally I tried to follow the following rules:

If the warning check is about a function call use the column from the start of the module name |Enum.count(...)
If the warning check is about an operator use the operator start for the column(x |and x instead of the right/left hand side)
If the warning check is an erlang function use the column from the function (erlang.|function since the AST for the erlang bit is different and has no column information)

rrrene

This is looking great. Just a minor nitpick and we have to find out why the column info seems to be off for Elixir versions 1.11 and 1.12 ...

rrrene · 2024-04-29T20:02:59Z

lib/credo/check/warning/application_config_in_module_attribute.ex

  end

  defp get_forbidden_call(ast, acc) do
    {ast, acc}
  end

-  defp issues_for_call(attribute, {call, trigger}, meta, issue_meta, issues) do
+  defp issues_for_call(attribute, {call_meta, call, trigger}, _meta, issue_meta, issues) do


If we are ignoring _meta, we should probably not pass it on line 45.

NJichev · 2024-04-29T22:33:26Z

So diffing elixir 1.13 and 1.12 doesn't reveal at first glance what changed between the two versions.
Checking the test examples I found that all the errors are coming from function calls coming from the erlang std library, i.e:

:erlang.open_port(...)
:os.cmd(...)

rrrene · 2024-04-30T20:20:17Z

Yeah. I just pushed some "checks" to the testing pipeline that make sure the trigger, line and column info is actually consisistent.

Could you rebase against master?

rrrene · 2024-04-30T21:27:07Z

Okay, so the new pipeline is showing some deeper issues: https://github.com/rrrene/credo/actions/runs/8901349869/job/24445604486?pr=1127

We have to provide an accurate column without changing the trigger (the trigger being the String to be highlighted by tooling like an editor).

NJichev · 2024-04-30T23:06:32Z

Hmm I don't think I've touched any of the trigger code, I'll take a look tomorrow morning

rrrene · 2024-05-01T10:25:54Z

I probably did not phrase that in a good way, apologies.

The trigger is the string triggering the issue, e.g. ":os.cmd". The column has to be the start of that string in the source code.

This PR wants to fix instances where the column is wrong when there are two instances of the same call on the same line, which is super good 👍

But we cannot pick the column arbitrarily, because it has to be the actual start of the trigger 🙂

What I meant with my last comment is that it is not an option to change the trigger from ":os.cmd" to "cmd", because that would potentially break backwards compatibility and the intent of the check in question.

NJichev · 2024-05-01T12:34:56Z

Maybe in this case I could get the module name and calculate it's string length and remove that from the column length to fix it, but it still doesn't cover the case where the AST is off by 1 on older versions of Elixir.
I could either compile a different version of the code for older versions to make it consistent if you think that's okay?
Another approach would be to just make this elixir version check in the tests and leave it like that?

rrrene · 2024-05-01T14:56:52Z

Let's ignore the off-by-one thing for Elixir 1.11 and 1.12 for now (I can deal with that after merging).

Our main focus should be to get the correct column for the existing trigger, as that is the primary improvement this PR brings 👍

NJichev · 2024-05-01T17:18:21Z

Hey I fixed the Erlang stdlib function columns by just calculating the string length of the module. Also fixed some of the triggers that were producing warnings to match the real trigger.

Do you have any suggestions on how to fix the other 3 triggers that mismatch the actual code and column that are producing the warnings in the tests:

Enum empty check - here I return the column to be the start of the operator, I can make it be the Enum.count or length there and that would make it match the trigger.
Missing metadata key in logger config - so the column produced here is the start of the logger statement Logger.info(..., key: :foo) while the trigger is just "key" in this example - here I'm not exactly sure what's the most appropriate.
Forbidden Module check - here the warning comes from grouped aliases while the trigger itself is the fully qualified module name, so this warning won't be easily fixable, maybe remove the fully qualified name from the trigger, but that could be confusing as well.

So the last 2 can maybe be left as is. The empty enum check I've left it to follow the operator semantics above, but am open to changing.

rrrene · 2024-05-01T20:08:06Z

So the last 2 can maybe be left as is.

What do you mean? We can not include a wrong column that is not the start of the trigger in the source code.

In your first example, the trigger is actually Enum.count and not some kind of operator (I would also say that is the case from a educational point of view: the problem is not that you are using ==, the problem is that you are using Enum.count when you don't need to).

In the second example, of course the problem is the metadata entry in the keyword list (named :key in the test, not a good name I have to admit) and not the call to a Logger function.

Of course, in the special case with the grouped aliases for the Forbidden Module check, the trigger is actually wrong, because the string does not appear in the source code. So we have to change the (incorrect) trigger in this case (or you could simply revert your changes to this check and we deal with it in the future).

NJichev · 2024-05-02T00:39:00Z

Hey 👋
I adjusted the expensive enum check column to get it from the correct operator side instead of using the line;col of the operator. Also fixed the trigger for the forbidden module to be the short name when in a group alias since the trigger is wrong otherwise and won't match the source code(did a small refactor there).

Decided to revert the column info for the missing metadata key in the Logger config since it's not so trivial to calculate the right column info for the starting position of the key in question. Maybe I could do it in a separate PR by using Macro.to_string instead of doing it by hand on the AST.

rrrene

We are getting there. With one or two of the "non-necessary refactorings" I am afraid that this PR got worse, so let's discuss/revert 👍

rrrene · 2024-05-02T15:02:45Z

lib/credo/check/warning/leaky_environment.ex

@@ -27,28 +27,37 @@ defmodule Credo.Check.Warning.LeakyEnvironment do
    Credo.Code.prewalk(source_file, &traverse(&1, &2, issue_meta))
  end

+  @offset 2


Let's give this a less confusing name: @colon_and_dot_length

I saw that there's a backports module, should I move the attribute there as a function and compile a different version for the older elixir versions to make the tests pass?

As I mentioned I will deal with that later. Especially because it won't be a problem for long, since we can stop supporting 1.12 as soon as Elixir 1.17 drops.

rrrene · 2024-05-02T15:05:44Z

lib/credo/check/warning/forbidden_module.ex

-        Enum.map(modules, fn key -> {Name.full(key), nil} end)
+        Map.new(modules, fn module ->
+          full = Name.full(module)
+          {full, "The `#{Name.full(module)}` module is not allowed."}


Why did you choose to put the fallback message here instead of where it was? To me seems unclear.

Hmm I did it because issue_for was getting too many arguments since I would need the trigger to be the short name in some cases, and then I'd need to pass in the full name as well in this scenario. I can revert it though

I think we can tolerate 5, 6 or even 7 arguments in a function in functional programming 🙂

Especially if it makes the flow & origin of information more clear.

rrrene · 2024-05-02T15:06:43Z

lib/credo/check/warning/forbidden_module.ex


+  defp issue_for(issue_meta, meta, trigger, message) do


Again: Why the refactor to get the message (and trigger) as an argument instead of looking it up here?

The trigger would need to be different if it's coming from a group alias, while the message is always based on the full name so far, since I didn't know if changing the message would be some kind of a compatibility problem decided to keep it consistent as it was. But I don't mind reworking it like this if you prefer?

If the trigger is different for different scenarios, we can pass it in.

rrrene · 2024-05-02T15:10:10Z

lib/credo/check/warning/unsafe_exec.ex

+  # offset 2 characters for the dot call and the atom syntax
+  @offset 2


See above: please rename and delete the comment explaining the name

rrrene · 2024-05-02T15:12:01Z

test/credo/check/warning/iex_pry_test.exs

+    """
+    |> to_source_file
+    |> run_check(@described_check)
+    |> assert_issues(fn [one, two] ->


Why are these list items named in order here and named in reverse order on many other tests? Please be consistent.

rrrene · 2024-05-02T15:12:31Z

test/credo/check/warning/mix_env_test.exs

+    """
+    |> to_source_file
+    |> run_check(@described_check)
+    |> assert_issues(fn [one, two] ->


rrrene · 2024-05-02T15:16:09Z

lib/credo/check/warning/forbidden_module.ex

+      Enum.reduce(aliases, issues, fn {:__aliases__, meta, module}, issues ->
+        full_name = Name.full([base_alias, module])
+
+        if found_module?(full_name, forbidden_modules) do


Isn't this exactly the same as if forbidden_modules[full_name] do?

It's an artifact of the map initializing with nil values at first, while is_map_key would return true here.

NJichev · 2024-05-02T20:33:34Z

Reverted the changes on the forbidden module check and just added a trigger as a variable + reduce one pass with enum.

Refs #1127

rrrene · 2024-05-04T19:32:21Z

@NJichev Thx for this! It highlighted several pain points in Credo's codebase 👍

FYI: I tried to simplify your PR further when merging on the command line: ba4f622

NJichev · 2024-05-04T19:32:36Z

Thanks for guiding me through ❤️

rrrene reviewed Apr 29, 2024

View reviewed changes

NJichev added 3 commits April 30, 2024 23:43

Add correct column metadata to all warning checks

f38a299

Remove unused argument

db720f8

Fix formatting

47b2000

NJichev force-pushed the add-correct-column-reporting-to-warning-checks branch from b1ef181 to 47b2000 Compare April 30, 2024 20:43

Fix column length for erlang stdlib functions

346953b

NJichev added 2 commits May 2, 2024 03:23

Fix triggers for forbidden module and empty enum

4f37ddb

Revert missing metadata key column metadata

d8011ee

rrrene reviewed May 2, 2024

View reviewed changes

NJichev added 4 commits May 2, 2024 19:22

Rename module attribute

450f9e9

Make issue assertions consistent

636cd9d

Remove found_module?/2

cdee25f

Revert refactor and just pass the trigger

7a4d7d9

rrrene added a commit that referenced this pull request May 4, 2024

Rework last merge to work with older Elixir versions

ba4f622

Refs #1127

rrrene merged commit cdfd789 into rrrene:master May 4, 2024
16 of 21 checks passed

NJichev deleted the add-correct-column-reporting-to-warning-checks branch May 7, 2024 18:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add correct column metadata to all warning checks #1127

Add correct column metadata to all warning checks #1127

NJichev commented Apr 29, 2024

rrrene left a comment

rrrene Apr 29, 2024

NJichev commented Apr 29, 2024

rrrene commented Apr 30, 2024

rrrene commented Apr 30, 2024

NJichev commented Apr 30, 2024

rrrene commented May 1, 2024

NJichev commented May 1, 2024

rrrene commented May 1, 2024

NJichev commented May 1, 2024

rrrene commented May 1, 2024

NJichev commented May 2, 2024 •

edited

Loading

rrrene left a comment

rrrene May 2, 2024

NJichev May 2, 2024

rrrene May 2, 2024

rrrene May 2, 2024

NJichev May 2, 2024

rrrene May 2, 2024

rrrene May 2, 2024

NJichev May 2, 2024

rrrene May 2, 2024

rrrene May 2, 2024

rrrene May 2, 2024

rrrene May 2, 2024

rrrene May 2, 2024

NJichev May 2, 2024 •

edited

Loading

NJichev commented May 2, 2024

rrrene commented May 4, 2024

NJichev commented May 4, 2024

		# offset 2 characters for the dot call and the atom syntax
		@offset 2

Add correct column metadata to all warning checks #1127

Add correct column metadata to all warning checks #1127

Conversation

NJichev commented Apr 29, 2024

rrrene left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NJichev commented Apr 29, 2024

rrrene commented Apr 30, 2024

rrrene commented Apr 30, 2024

NJichev commented Apr 30, 2024

rrrene commented May 1, 2024

NJichev commented May 1, 2024

rrrene commented May 1, 2024

NJichev commented May 1, 2024

rrrene commented May 1, 2024

NJichev commented May 2, 2024 • edited Loading

rrrene left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NJichev May 2, 2024 • edited Loading

Choose a reason for hiding this comment

NJichev commented May 2, 2024

rrrene commented May 4, 2024

NJichev commented May 4, 2024

NJichev commented May 2, 2024 •

edited

Loading

NJichev May 2, 2024 •

edited

Loading